Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix issue when the storage class of a persistent volume is empty and add test #6615

Merged
merged 2 commits into from
Jun 9, 2020
Merged

Fix issue when the storage class of a persistent volume is empty and add test #6615

merged 2 commits into from
Jun 9, 2020

Conversation

Simwar
Copy link
Contributor

@Simwar Simwar commented May 11, 2020

What does this PR do?

Fix an issue when the storage class of a persistent volume is empty.
Adds a test for this use case.

Motivation

Customer reported this issue because some of their persistent volumes did not have a storage class.
This can happen if persistent volumes are restored.

Additional Notes

Review checklist (to be filled by reviewers)

  • Feature or bugfix MUST have appropriate tests (unit, integration, e2e)
  • PR title must be written as a CHANGELOG entry (see why)
  • Files changes must correspond to the primary purpose of the PR as described in the title (small unrelated changes should have their own PR)
  • PR must have changelog/ and integration/ labels attached

@Simwar Simwar requested review from a team as code owners May 11, 2020 11:40
FlorianVeaux
FlorianVeaux reviewed May 11, 2020
View reviewed changes
Copy link
Member

@FlorianVeaux FlorianVeaux left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Such a PR means that some metrics won't have the storageclass tag. I think this can lead to some query issues when trying to group by the storage class or when summing over all the storage classes.

Is the issue that the tag appears as "storageclass:" on Datadog? What do you think of renaming the tag value to "unset" maybe?

Beside that higher level concern, the PR looks good

@Simwar
Copy link
Contributor Author

Simwar commented May 19, 2020

Hello @FlorianVeaux

Thanks for the review!
From the test values, I believe that there will be no tag storageclass set at all on the metric.
So yes, what you are saying makes sense.
Is it something that we see in other integrations? Setting the value to unset or even none?

@codecov
Copy link

codecov bot commented Jun 8, 2020

Codecov Report

Merging #6615 into master will increase coverage by 0.65%.
The diff coverage is 100.00%.

Impacted Files Coverage Δ
...atadog_checks/kubernetes_state/kubernetes_state.py 83.47% <100.00%> (+0.88%) ⬆️
kubernetes_state/tests/test_kubernetes_state.py 97.16% <100.00%> (+0.01%) ⬆️
...ivemq_xml/datadog_checks/activemq_xml/__about__.py
tomcat/tests/test_check.py
...v/datadog_checks/dev/tooling/configuration/spec.py
datadog_checks_base/tests/test_serialization.py
process/datadog_checks/process/process.py
mapreduce/datadog_checks/mapreduce/mapreduce.py
...cs_fargate/datadog_checks/ecs_fargate/__about__.py
..._checks_base/datadog_checks/base/utils/tailfile.py
... and 811 more

@ahmed-mez ahmed-mez merged commit 66149a4 into master Jun 9, 2020
@ahmed-mez ahmed-mez deleted the ksm-persistent-volume-fix branch June 9, 2020 09:08
@AlexandreYang AlexandreYang changed the title added test for persistent volume without a storage class + fix Added test for persistent volume without a storage class + fix Jun 29, 2020
@AlexandreYang AlexandreYang changed the title Added test for persistent volume without a storage class + fix Fix an issue when the storage class of a persistent volume is empty and add test Jun 29, 2020
@AlexandreYang AlexandreYang changed the title Fix an issue when the storage class of a persistent volume is empty and add test Fix issue when the storage class of a persistent volume is empty and add test Jun 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FlorianVeaux FlorianVeaux FlorianVeaux approved these changes

@ahmed-mez ahmed-mez ahmed-mez approved these changes

Assignees
No one assigned
Labels
containers integration/kubernetes_state
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Simwar @FlorianVeaux @ahmed-mez